Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet] Add balance and contact tracking #4209

Merged
merged 4 commits into from
Jun 25, 2020
Merged

[Wallet] Add balance and contact tracking #4209

merged 4 commits into from
Jun 25, 2020

Conversation

annakaz
Copy link
Contributor

@annakaz annakaz commented Jun 25, 2020

Description

The new wallet KPIs require a few analytics we weren't already tracking. This PR adds balance and contact imports tracking, which is the final addition to be able to calculate the KPIs listed here.

Note that transaction size analytics will be calculated using eksportisto data

Tested

Not tested, no logic change- just adding analytics

Related issues

Backwards compatibility

Yes

@annakaz annakaz added the automerge Have PR merge automatically when checks pass label Jun 25, 2020
@annakaz annakaz requested review from tarikbellamine, nambrot and a team June 25, 2020 00:51
Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment

CustomEventNames.fetch_balance,
token === CURRENCY_ENUM.DOLLAR
? {
dollarBalance: balance,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

balance is a big number, i think we want a string or number representation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it might be nice to just have a balance field and then a separate currency field. Makes empty columns less prevalent

Copy link
Contributor

@i1skn i1skn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit e74c778 into master Jun 25, 2020
@mergify mergify bot deleted the annakaz/kpis branch June 25, 2020 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants